-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Different handling label analysis if result already calculated #21
Conversation
Codecov Report
@@ Coverage Diff @@
## main #21 +/- ##
==========================================
- Coverage 98.48% 98.48% -0.01%
==========================================
Files 364 364
Lines 26797 26818 +21
==========================================
+ Hits 26392 26412 +20
- Misses 405 406 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
This change has been scanned for critical changes. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I entirely understand the workflow here but the code changes look OK to me.
So the CLI will PATCH the label analysis w/ additional labels and that triggers off another task that presumably hits this LabelAnalysisRequestState.FINISHED
conditional. In that case we just combine the previous results with the calculated results for the new labels. Is that roughly correct?
This change is intended to work with the PATCHing of labels by the CLI after collecting them. Currently doing that doesn't trigger another task in the worker. SO if the labels arraive after we finished calculating we are left with incomplete results on the database. That is fine as the CLI will calculate again. I think this change might have been a bit premature, but the idea is to have a different way of handling the calculation if we already have results. Then we simply calculate the final result using the saved labels and add in the requested labels that we might not have had the first time around. This would be much much faster than calculating everything again.
Updating branch and making sure all tests are OK.
1d26d8f
to
03509a2
Compare
Yes, that's the idea. CLI sends a POST request to api that creates the original LabelAnalysisRequest instance and triggers this task. Initially this has no Later on - after collecting the labels present - the CLI will PATCH the labels in the api, but (right now) the task will not be re-triggered. This is not a problem for using ATS because the CLI can sort out what labels to include / remove locally. The thing is that we want to have metrics around how many tests ATS is saving. For that reason we need to re-trigger the LabelAnalysis task with the larq that has already been calculated and update the result.
|
From codecov/worker#21 we have a different way of handling updates for `LabelAnalysisRequest` objects. This is important because we want to use these objects to extract metrics from ATS. However to actually have the updated values we need to trigger the task again. These changes trigger the task again :3 closes codecov/engineering-team#456
From codecov/worker#21 we have a different way of handling updates for `LabelAnalysisRequest` objects. This is important because we want to use these objects to extract metrics from ATS. However to actually have the updated values we need to trigger the task again. These changes trigger the task again :3 closes codecov/engineering-team#456
This change is intended to work with the PATCHing of labels by the CLI after collecting them.
Currently doing that doesn't trigger another task in the worker. SO if the labels arraive after we finished calculating we are left with incomplete results on the database. That is fine as the CLI will calculate again.
I think this change might have been a bit premature, but the idea is to have a different way of handling the calculation if we already have results. Then we simply calculate the final result using the saved labels and add in the requested labels that we might not have had the first time around. This would be much much faster than calculating everything again.
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.